-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 2D-SWE and move wet/dry functionality from Trixi.jl #18
Add 2D-SWE and move wet/dry functionality from Trixi.jl #18
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
===========================================
- Coverage 100.00% 99.25% -0.75%
===========================================
Files 4 32 +28
Lines 85 941 +856
===========================================
+ Hits 85 934 +849
- Misses 0 7 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just formatting or some text updates to the docstrings. I only left two questions regarding how the min_max_speed_chen_noelle
dispatches within the new package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things are looking very nice! Just one small changes that I think is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Fantastic work @patrickersing! I think we can merge this and continue onto adding in the two-layer equations. Do you want one last look @sloede ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me and nearly ready to merge. I've just left a few suggestions, most of which are food for thought and could be addressed later.
Great job, @patrickersing, I am looking forward to seeing TrixiShallowWater.jl as a registered package :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great job, @patrickersing!
Great, @andrewwinters5000 then we should be ready to merge! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are good to go!
This PR introduces the 2D shallow water equations and the wet/dry functionality, that is to be removed from
Trixi.jl
in trixi-framework/Trixi.jl#1809. The two-layer SWE, that are also removed will be introduced in a separate PR.One notable change in this PR is that I changed from
using Trixi: Trixi
=>using Trixi
. While moving more code toTrixiShallowWater.jl
, I noticed that it is becoming to cumbersome and hard to read when we qualify every symbol fromTrixi.jl
, especially when using repetitive basic functionalities such asnnodes
oreachelement
.With this change we now only need to qualify functions that will be extended by
TrixiShallowWater.jl
or when accessing functions that are not exported byTrixi.jl
.Besides that most of the code changes are just copied over from
Trixi.jl
and adapted to the new package.The PR should only be merged after removing these features from
Trixi.jl
in trixi-framework/Trixi.jl#1809Reminder: We will need to bump the compat bounds in the
Project.toml
for Trixi to 0.7 once it is released.